-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Db monitor #52
Db monitor #52
Conversation
Sorry for the delay @hover2pi, I'll look at this tomorrow or Friday |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @hover2pi, very nicely written. I have a few questions and requests, but the code is shaping up nicely.
jwql/dbmonitor/dbmonitor.py
Outdated
>>> inventory, keywords = dbmonitor.jwst_inventory() | ||
|
||
""" | ||
import sys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hover2pi Please order imports based on the PEP 8 suggestion of three groups (and in alphabetical order)
Imports should be grouped in the following order:
1. standard library imports
2. related third party imports
3. local application/library specific imports
You should put a blank line between each group of imports.
jwql/dbmonitor/dbmonitor.py
Outdated
from astroquery.mast import Mast | ||
import astropy.table as at | ||
|
||
JWST_INSTRUMENTS = ['NIRISS', 'NIRCam', 'NIRSpec', 'MIRI', 'FGS'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hover2pi It would probably be a good idea to stick global lists like these in the utils.py
module and import them. Then they could easily be used by other modules. Could you put those in there?
jwql/dbmonitor/dbmonitor.py
Outdated
'EVENTLIST', 'CUBE', 'CATALOG', 'ENGINEERING', 'NULL'] | ||
|
||
|
||
def listMissions(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hover2pi Per our style guide, could you order the function definitions in alphabetical order?
jwql/dbmonitor/dbmonitor.py
Outdated
return_data: bool | ||
Return the actual data instead of counts only | ||
|
||
Returns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hover2pi You might want to add a note here that all results are returned if return_data
is True
jwql/dbmonitor/dbmonitor.py
Outdated
{'paramName': 'dataproduct_type', 'values': dataproduct}] | ||
|
||
# Include additonal filters | ||
for k, v in add_filters.items(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hover2pi PEP 8 discourages single letter variable names, but this naming convention seems like somewhat of a standard amongst the python community, so I'll allow it! I'm only mentioning this because I have given @bhilbert4 grief about this before and I want to play fair ;)
jwql/dbmonitor/dbmonitor.py
Outdated
return request | ||
|
||
|
||
def mastQuery(request): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hover2pi Could you instead use the MAST API directly, instead of having to establish connections and sending POST
s? i.e.:
response = astroquery.mast.Mast.service_request_async('Mast.Caom.Filtered', params)
result = resonse[0].json()
Similar to how we are querying MAST in this notebook. It seems simpler to me.
jwql/dbmonitor/dbmonitor.py
Outdated
count = instrument_inventory(instruments, dataproduct=dataproducts) | ||
inventory.add_row(['*', '*', count]) | ||
|
||
# Retrieve one dataset to get header keywords |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hover2pi It will be nice to have an understanding of these keywords that come out of the Mast.Caom.Filtered
service, but I would also like to be tracking the number of keywords that come out of the instrument specific services (e.g. Mast.Jwst.Filtered.Niriss
). Could that be added?
…modapi generated docs for now. #51
first pass at getting sphinx and automodapi running
Filename parser utility function
Ok @bourque , this should be ready to go. I'm going to add the plots in the Web app in a separate branch. |
@bourque I fixed some docstrings, modified the queries to accept filters (just in case), and broke out the keyword queries into their own functions so feeding it to the Web app is easier. |
Thanks @hover2pi, I'm going to try to review this today or tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hover2pi This looks and works great, and exactly what I had in mind.
My only comment is that it seems the jwst_inventory
and jwst_inventory_caom
functions are mostly the same and do not follow the DRY principle (Don't Repeat Yourself) very well. I wonder if it would be better to add a parameter called service
and use conditions to distingish the calls to the instrument_inventory
functions, i.e.
def jwst_inventory(service, instruments=JWST_INSTRUMENTS, dataproducts=['image', 'spectrum', 'cube'],
plot=False):
...
if service == 'filtered':
count = instrument_inventory(instrument, dataproduct=dp)
elif service == 'caom':
count = instrument_inventory_caom(instrument, dataproduct=dp)
...
if service == 'filtered':
keywords[instrument] = instrument_keywords(instrument)
elif service == 'caom':
keywords[instrument] = instrument_keywords(instrument)
...
What do you think?
jwql/utils/utils.py
Outdated
|
||
JWST_INSTRUMENTS = ['NIRISS', 'NIRCam', 'NIRSpec', 'MIRI', 'FGS'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hover2pi Oops, somehow this got defined twice. Could you remove this?
@bourque I combined the CAOM and filtered queries and removed that weird duplicate global variable definition. Should be good to go. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im not actually approving this now, just trying something out
@hover2pi Random question that may or may not have to do with our |
…ing it. Using bokeh.charts instead of bkcharts.
@hover2pi This is good to go! Great work. I made a few additional commits to this, namely to save the plots to the Also, I had to switch to using Anyways, this is a good working version, so I am going to merge it. I can already think of a few ways to improve these charts, but we can deal with that in a separate PR. |
Wrote database monitor to count files in MAST #48